child_process: watch child_process stdin pipe peer close event#62353
child_process: watch child_process stdin pipe peer close event#62353Tseian wants to merge 4 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
c++
Tseian
changed the title
Tseian
force-pushed
the
main
branch
3 times, most recently
from
071fa18 to
4cedb07
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62353 +/- ##
==========================================
- Coverage 89.68% 89.68% -0.01%
==========================================
Files 676 676
Lines 206710 206772 +62
Branches 39584 39609 +25
==========================================
+ Hits 185398 185438 +40
- Misses 13441 13444 +3
- Partials 7871 7890 +19
🚀 New features to boost your workflow:
|
Tseian
force-pushed
the
main
branch
2 times, most recently
from
0ae2360 to
09463c0
Compare
Tseian
changed the title
Tseian
force-pushed
the
main
branch
2 times, most recently
from
9e25a20 to
78acd67
Compare
Tseian
changed the title
Tseian
force-pushed
the
main
branch
3 times, most recently
from
e704d96 to
8cc7608
Compare
Tseian
changed the title
|
@nodejs-github-bot retest |
|
@jasnell @ronag @jbunton-atlassian Hi everyone, Could you please have a code review for this PR? |
|
@jasnell Please have a code review again for this PR.
|
addaleax
added
the
child_process
addaleax
added
the
semver-major
| PipeWrap::~PipeWrap() { | ||
| peer_close_watching_ = false; | ||
| peer_close_cb_.Reset(); | ||
| } |
There was a problem hiding this comment.
This doesn't do anything/will just be optimized out by the compiler anyway
| static void Fchmod(const v8::FunctionCallbackInfo<v8::Value>& args); | ||
|
|
||
| bool peer_close_watching_ = false; | ||
| v8::Global<v8::Function> peer_close_cb_; |
There was a problem hiding this comment.
nit/optional: I'd consider using an internal field over a Global. The only reason this doesn't create a memory leak is because PipeWrap instances are always strong references, and that will probably keep being true, but it's still an extra assumption we don't really need.
| } | ||
|
|
||
| wrap->peer_close_watching_ = false; | ||
| wrap->peer_close_cb_.Reset(); |
There was a problem hiding this comment.
Doesn't an empty peer_close_cb_ always correspond to peer_close_watching_ being false? That means we're keeping the same state twice
| errors::TryCatchScope try_catch(env); | ||
| try_catch.SetVerbose(true); |
There was a problem hiding this comment.
Shouldn't be necessary, right? We don't do this for other callbacks from libuv events either
| if (watchPeerClose && | ||
| process.platform !== 'win32' && | ||
| typeof pipe?.watchPeerClose === 'function') { | ||
| pipe.watchPeerClose(true, () => sock.destroy()); | ||
| sock.once('close', () => pipe.watchPeerClose(false)); | ||
| } |
There was a problem hiding this comment.
This is quite a bit of extra code, but wouldn't just adding an unconditional sock.resume(); give the same result? That way we keep reading (but not consuming) data from the socket until we hit EOF, and since allowHalfOpen isn't set, that should also close the writable side of the stream. In other words, if I'm not missing anything (and which is completely possible), this PR can be shortened to:
| if (watchPeerClose && | |
| process.platform !== 'win32' && | |
| typeof pipe?.watchPeerClose === 'function') { | |
| pipe.watchPeerClose(true, () => sock.destroy()); | |
| sock.once('close', () => pipe.watchPeerClose(false)); | |
| } | |
| socket.resume(); |
(plus a comment or so about 'why' we do that, ofc)
4 participants
Issue
#25131
Description
Watch pipe peer close(EOF/HUP) event, only support for unix. Once the event is detected, a JS callback is triggered to execute, which in turn triggers a method to destroy the socket. Eventually child_process.stdin.on('close') will be triggered.
Before
child_process.stdin.on('close') will not be triggered if the readable end of the pipe has been closed.
After
child_process.stdin.on('close') will be triggered if the readable end of the pipe has been closed.
Changes
Test